Atc and ux#164
Merged
Merged
Conversation
The ADT backend returns the priority the same way as SAPGUI now. This commit reverts 07cae87
I used a user without S_DEVELOP and that user cannot create new class. ADT backend sends the discovery collection for classes with the element accept holding an empty string. The empty string was added as the accepted mime-type for oo/classes and the error output was highly confusing: Not supported mimes: not in application/vnd.sap.super.cool.txt+xml;application/vnd.sap.super.cool.txt.v2+xml The new message will be: Not supported mimes for awesome/success: "application/something.else+xml" not in "application/vnd.sap.super.cool.txt+xml;application/vnd.sap.super.cool.txt.v2+xml"
For some reason, in the case your use does not have S_DEVELOP the ADT backend sends collections XML in the discovery response with empty an <accept> tag instead of leaving it out completely. Without this patch, sapcli believes that ADT expects empty string in the HTTP header Content-Type for data of the corresponding object. This patch adds ignoring of empty accepts which leads into no mime type registration. That leads into use of the default mime type assigned to an ADT object mapper. And that should lead into ADT backend error if sapcli sends a request for modification/creation of such an object type. I do not want to add a permissions evaluation engine into sapcli - the ABAP side must handle it correctly because otherwise we would have a security hole in the backend.
In the case the class.create() fails, we go to finally and call class.delete() which will probably fail on its own exception because the object most probably does not exist.
📝 WalkthroughWalkthroughThe PR removes ATC result post-processing, filters empty MIME types during ADT discovery parsing, improves MIME type error messages with object context, and prevents ABAP class deletion failures from overriding primary execution results via warning emission instead of exception propagation. ChangesBug fixes and error handling improvements
🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/unit/test_sap_adt_object.py (1)
242-243: ⚡ Quick winFix continuation line indentation.
The string continuation at line 243 is under-indented according to PEP 8 style guidelines and should align with the opening quote.
♻️ Proposed fix
self.assertEqual(str(caught.exception), - 'Not supported mimes for awesome/success: "application/something.else+xml" not in "application/vnd.sap.super.cool.txt+xml;application/vnd.sap.super.cool.txt.v2+xml"') + 'Not supported mimes for awesome/success: "application/something.else+xml" not in "application/vnd.sap.super.cool.txt+xml;application/vnd.sap.super.cool.txt.v2+xml"')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/test_sap_adt_object.py` around lines 242 - 243, Adjust the continuation indentation of the long expected string in the assertion so it aligns with the opening quote of the first line of the self.assertEqual call; locate the self.assertEqual(str(caught.exception), ...) in test_sap_adt_object.py and re-indent the continued line containing 'Not supported mimes for awesome/success: "application/something.else+xml" ...' to match the opening quote position to satisfy PEP 8 string continuation alignment.test/unit/test_sap_adt_connection.py (1)
315-326: ⚡ Quick winFix continuation line indentation.
The list continuation starting at line 316 is over-indented according to PEP 8 style guidelines.
♻️ Proposed fix
- self.assertEqual(list(self.connection.collection_types.keys()), [ - '/sap/bc/adt/bopf/businessobjects', - '/sap/bc/adt/packages', - '/sap/bc/adt/functions/groups/{groupname}/fmodules', - '/sap/bc/adt/functions/groups/{groupname}/includes', - '/sap/bc/adt/functions/groups', - '/sap/bc/adt/ddic/ddl/formatter/identifiers', - '/sap/bc/adt/sadl/gw/mde', - '/sap/bc/adt/quickfixes/evaluation', - '/sap/bc/adt/wdy/views', - '/sap/bc/adt/stub/test_accept', - ]) + self.assertEqual(list(self.connection.collection_types.keys()), [ + '/sap/bc/adt/bopf/businessobjects', + '/sap/bc/adt/packages', + '/sap/bc/adt/functions/groups/{groupname}/fmodules', + '/sap/bc/adt/functions/groups/{groupname}/includes', + '/sap/bc/adt/functions/groups', + '/sap/bc/adt/ddic/ddl/formatter/identifiers', + '/sap/bc/adt/sadl/gw/mde', + '/sap/bc/adt/quickfixes/evaluation', + '/sap/bc/adt/wdy/views', + '/sap/bc/adt/stub/test_accept', + ])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/test_sap_adt_connection.py` around lines 315 - 326, The list literal passed to self.assertEqual comparing list(self.connection.collection_types.keys()) is over-indented; fix by adjusting the continuation indentation so the items either align under the opening bracket or use a 4-space hanging indent per PEP 8. Update the list starting at the call to self.assertEqual (the bracketed list of paths) so each string element lines up consistently with the chosen style and remove the extra indent before '/sap/bc/adt/stub/test_accept'.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/unit/test_sap_adt_connection.py`:
- Around line 315-326: The list literal passed to self.assertEqual comparing
list(self.connection.collection_types.keys()) is over-indented; fix by adjusting
the continuation indentation so the items either align under the opening bracket
or use a 4-space hanging indent per PEP 8. Update the list starting at the call
to self.assertEqual (the bracketed list of paths) so each string element lines
up consistently with the chosen style and remove the extra indent before
'/sap/bc/adt/stub/test_accept'.
In `@test/unit/test_sap_adt_object.py`:
- Around line 242-243: Adjust the continuation indentation of the long expected
string in the assertion so it aligns with the opening quote of the first line of
the self.assertEqual call; locate the self.assertEqual(str(caught.exception),
...) in test_sap_adt_object.py and re-indent the continued line containing 'Not
supported mimes for awesome/success: "application/something.else+xml" ...' to
match the opening quote position to satisfy PEP 8 string continuation alignment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e57a3ee9-88db-4a2a-9025-b9173a180661
📒 Files selected for processing (9)
sap/adt/atc.pysap/adt/core.pysap/adt/objects.pysap/platform/abap/run.pytest/unit/fixtures_adt.pytest/unit/test_sap_adt_atc.pytest/unit/test_sap_adt_connection.pytest/unit/test_sap_adt_object.pytest/unit/test_sap_platform_abap_run.py
💤 Files with no reviewable changes (1)
- sap/adt/atc.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.